bpo-29573: Fixed unlinking already removed NamedTemporaryFile.#134
bpo-29573: Fixed unlinking already removed NamedTemporaryFile.#134andrewnester wants to merge 1 commit intopython:masterfrom andrewnester:29573-named-temp-file
Conversation
briancurtin
left a comment
briancurtin
left a comment
There was a problem hiding this comment.
Looks fine to me, just needs a small test change.
Lib/test/test_tempfile.py
Outdated
There was a problem hiding this comment.
Should add a self.addCleanup on that tmpdir—see the test above (test_bad_mode) for an example, just to make sure that if this test ever does fail that we don't have a temp dir that doesn't get cleaned up by us.
There was a problem hiding this comment.
@briancurtin could you please review it again, it's fixed now
| tmpdir = tempfile.mkdtemp() | ||
| self.addCleanup(support.rmtree, tmpdir) | ||
| with tempfile.NamedTemporaryFile(delete=True, dir=tmpdir) as fp: | ||
| os.system('rm {}'.format(fp.name)) |
There was a problem hiding this comment.
Why does this use os.system to call rm, rather than calling os.unlink? 'rm' is not exactly cross-platform. This test will also fail on Windows (and other systems where you can't delete files that are currently in use). It'll have to be skipped on those platforms.
Replace the direct access to PyObject members with the preferred macros.
|
To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request. If/when the requested changes have been made, please leave a comment that says, |
serhiy-storchaka
changed the title
Replace the direct access to PyObject members with the preferred macros. (cherry picked from commit c7dcca5)
|
@briancurtin @Yhg1s @brettcannon Is this PR worth being revived? The user's repo does not exist any more. I could take care of it by creating a new PR if you want |
|
@flavianh up to you if you want to create a new PR to try and solve the problem. |
6 participants
Fix for http://bugs.python.org/issue29573
https://bugs.python.org/issue29573